Deprecate operator & for getting addresses#10206
Deprecate operator & for getting addresses#10206aidanfnv wants to merge 11 commits intoshader-slang:masterfrom
& for getting addresses#10206Conversation
📝 WalkthroughWalkthroughThis PR adds validation to reject the address-of operator ( Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
For clarification, is this deprecating overloading support for this particular operator, or just addressing of arbitrary L-values? Full removal would be a bit unfortunate since it's not that uncommon that I write code like |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/diagnostics/invalid-constant-pointer-taking.slang (1)
25-31:⚠️ Potential issue | 🟡 MinorDon’t lock in cascading diagnostics after E30087.
Line 25 is now using an operator that is unconditionally rejected, so lines 28-31 make the test require secondary immutable/l-value diagnostics from an operator path that should ideally stop after E30087. Please either short-circuit after emitting E30087 and remove these old expectations, or make this test only assert the intended unsupported-operator diagnostic. As per coding guidelines, “Check that expected outputs match the intended behavior, not just current behavior.”
Proposed test expectation cleanup
float* ptr1 = &constant_float_buffer[threadId.x]; //diag: ^ address-of operator not supported //diag: ^ the '&' operator for taking addresses is no longer supported in Slang -//diag: ^ cannot take address of immutable object -//diag: ^ Not allowed to take the address of an immutable object -//diag: ^ argument must be l-value -//diag: ^ argument passed to parameter '0' must be l-value.
♻️ Duplicate comments (1)
source/slang/slang-check-expr.cpp (1)
3785-3802:⚠️ Potential issue | 🟠 MajorReturn an error after rejecting
operator&, and guard the callee lookup.This emits E30087 but then continues into the parameter validation loop, which can produce noisy follow-on diagnostics for an operator that is already unsupported. Also avoid dereferencing
getDecl()without checking theDeclRef, and use theKnownBuiltinAttribute*returned byfindModifierdirectly.Proposed fix
if (funcDeclRefExpr) { - auto knownBuiltinAttrDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr) - .getDecl() - ->findModifier<KnownBuiltinAttribute>(); - if (auto knownBuiltinAttr = as<KnownBuiltinAttribute>(knownBuiltinAttrDeclRef)) + auto funcDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr); + if (funcDeclRef) { - if (auto constantIntVal = as<ConstantIntVal>(knownBuiltinAttr->name)) + auto knownBuiltinAttr = + funcDeclRef.getDecl()->findModifier<KnownBuiltinAttribute>(); + if (knownBuiltinAttr) { - if (constantIntVal->getValue() == - (int)KnownBuiltinDeclName::OperatorAddressOf) + if (auto constantIntVal = as<ConstantIntVal>(knownBuiltinAttr->name)) { - getSink()->diagnose( - Diagnostics::AddressOfOperatorNotSupported{.expr = invoke}); + if (constantIntVal->getValue() == + (int)KnownBuiltinDeclName::OperatorAddressOf) + { + getSink()->diagnose( + Diagnostics::AddressOfOperatorNotSupported{.expr = invoke}); + return CreateErrorExpr(invoke); + } } } } }As per coding guidelines,
source/slang/**: “Null pointer safety and proper error handling via diagnostics.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4728a94c-a80a-436d-a68d-9e5b0690bdba
📒 Files selected for processing (4)
source/slang/slang-check-expr.cppsource/slang/slang-diagnostics.luatests/diagnostics/address-of-operator-deprecated.slangtests/diagnostics/invalid-constant-pointer-taking.slang
There was a problem hiding this comment.
Pull request overview
This PR removes support for the prefix & address-of operator in Slang by emitting a new unconditional diagnostic when the resolved callee is the built-in OperatorAddressOf, and adds regression tests to lock in the new behavior.
Changes:
- Add a new error diagnostic (E30087) indicating the
&address-of operator is no longer supported. - Emit E30087 during semantic checking of invocations resolved to the built-in address-of operator.
- Update/add diagnostics tests to validate the new error and its span message.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/diagnostics/invalid-constant-pointer-taking.slang | Updates expected diagnostics to include the new E30087 error for &. |
| tests/diagnostics/address-of-operator-deprecated.slang | Adds a focused diagnostic test ensuring & produces E30087. |
| source/slang/slang-diagnostics.lua | Defines the new address-of-operator-not-supported diagnostic (ID 30087). |
| source/slang/slang-check-expr.cpp | Adds semantic checking to diagnose invocations of the built-in address-of operator. |
| if (funcDeclRefExpr) | ||
| { | ||
| auto knownBuiltinAttrDeclRef = getDeclRef(m_astBuilder, funcDeclRefExpr) | ||
| .getDecl() | ||
| ->findModifier<KnownBuiltinAttribute>(); | ||
| if (auto knownBuiltinAttr = as<KnownBuiltinAttribute>(knownBuiltinAttrDeclRef)) | ||
| { | ||
| if (auto constantIntVal = as<ConstantIntVal>(knownBuiltinAttr->name)) | ||
| { | ||
| if (constantIntVal->getValue() == | ||
| (int)KnownBuiltinDeclName::OperatorAddressOf) | ||
| { | ||
| getSink()->diagnose( | ||
| Diagnostics::AddressOfOperatorNotSupported{.expr = invoke}); | ||
| } |
| @@ -0,0 +1,11 @@ | |||
| //DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute | |||
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ceaf9b39-4dfc-4cee-9bd0-e70c6e7fe483
📒 Files selected for processing (3)
source/slang/slang-check-expr.cpptests/diagnostics/address-of-operator-unsupported.slangtests/diagnostics/invalid-constant-pointer-taking.slang
| // CHECK: error[E30087]: address-of operator not supported | ||
| float* p = &buffer[threadId.x]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider also asserting the full diagnostic message.
The companion test invalid-constant-pointer-taking.slang shows the compiler emits two diagnostic lines at &: the short name address-of operator not supported and the full message the '&' operator for taking addresses is no longer supported in Slang. The CHECK here only verifies the short form, so a regression where the full user-facing message is dropped or altered would not be caught. A second CHECK line would lock that in.
♻️ Suggested addition
// CHECK: error[E30087]: address-of operator not supported
+ // CHECK: the '&' operator for taking addresses is no longer supported in Slang
float* p = &buffer[threadId.x];| float* mutablePtr1 = &mutable_float_buffer[threadId.x]; | ||
| //diag: ^ address-of operator not supported | ||
| //diag: ^ the '&' operator for taking addresses is no longer supported in Slang |
There was a problem hiding this comment.
Original test intent for & paths is no longer exercised.
The file header comment states the test exists to verify that Slang does not allow taking a pointer from StructuredBuffer/RWStructuredBuffer. With the new E30087 short-circuit on &, the mutablePtr1 (Line 14) and ptr1 (Line 25) cases now fail at operator resolution before the buffer-pointer semantics are ever checked — the previous mutability / l-value diagnostics on these lines are gone. The equivalent __getAddress cases (Lines 18, 29) still validate the original intent, so coverage is preserved there, but the & cases here are now effectively redundant with the new address-of-operator-unsupported.slang test.
Consider either (a) dropping the & cases from this file since the dedicated test already covers E30087, or (b) replacing them with __getAddress variants that still exercise the buffer-pointer rejection, to keep this file focused on its original purpose.
Also applies to: 25-27
There was a problem hiding this comment.
Verdict: 🔴 Has issues — 1 bug, 2 gaps
This PR adds an unconditional error (E30087) when the prefix & address-of operator is used, blocking it at the semantic checker level before IR generation. The approach is architecturally correct (fires in CheckInvokeExprWithCheckedOperands after overload resolution, applies uniformly to all backends). However, existing tests that rely on & reaching downstream diagnostics will break, and the error message lacks migration guidance.
Changes Overview
Semantic checker — address-of rejection (source/slang/slang-check-expr.cpp)
- What changed: Added a new check in
CheckInvokeExprWithCheckedOperandsthat detectsKnownBuiltinDeclName::OperatorAddressOfviaKnownBuiltinAttributeand emits E30087 before any parameter validation. ReturnsCreateErrorExprto halt further processing of the invocation.
Diagnostic definition (source/slang/slang-diagnostics.lua)
- What changed: Added error 30087 (
address-of-operator-not-supported) with message "the '&' operator for taking addresses is no longer supported in Slang".
Tests (tests/diagnostics/address-of-operator-unsupported.slang, tests/diagnostics/invalid-constant-pointer-taking.slang)
- What changed: New diagnostic test for E30087 on SPIRV target. Updated existing pointer-taking test to expect E30087 instead of previous immutability/l-value errors.
Findings (3 total)
| Severity | Location | Finding |
|---|---|---|
| 🔴 Bug | slang-check-expr.cpp:3797 |
Unconditional E30087 preempts E30019 in two existing tests (ptr-to-interface-local.slang, diagnose-ptr-to-interface-implicit-cast.slang) that are not updated in this PR |
| 🟡 Gap | slang-diagnostics.lua:1396 |
Error message doesn't mention replacement (__getAddress); user guide docs (03-convenience-features.md) show & usage that becomes incorrect |
| 🟡 Gap | address-of-operator-unsupported.slang:1 |
Test only covers SPIRV target with one pattern; missing local variable, struct member, and generic context coverage |
| if (knownBuiltinAttr) | ||
| knownBuiltinName = as<ConstantIntVal>(knownBuiltinAttr->name); | ||
|
|
||
| if (knownBuiltinName && |
There was a problem hiding this comment.
🔴 Bug: Unconditional E30087 will break existing tests that expect E30019
This check fires unconditionally for every operator& invocation and returns CreateErrorExpr(invoke) before the parameter validation loop. This preempts all downstream diagnostics — including E30019 (pointer type mismatch) — because the error expression propagates ErrorType through subsequent assignments, causing later type checks to short-circuit.
Two existing tests use & for address-of and expect E30019 errors that will now never be emitted:
-
tests/language-feature/dynamic-dispatch/ptr-to-interface-local.slang—IFoo* p = &local;expectsCHECK: error[E30019]. With this change, E30087 fires first and E30019 is never reached. Test will fail. -
tests/language-feature/dynamic-dispatch/diagnose-ptr-to-interface-implicit-cast.slang— Lines 39 (&f), 55 (&b), 59 (&ib) all use&for address-of. The test expects E30019 on subsequent conversion lines (44, 49, 54, 62), but since the&expressions now returnErrorType, the conversion checks that produce E30019 won't trigger.
Suggested fix: Update both test files to expect E30087 on the & lines and either remove or adjust the E30019 expectations accordingly. For diagnose-ptr-to-interface-implicit-cast.slang, consider splitting address-of tests from pointer-conversion tests so conversion diagnostics can be validated independently (e.g., construct Foo* via __getAddress instead of &).
| "address-of-operator-not-supported", | ||
| 30087, | ||
| "address-of operator not supported", | ||
| span { loc = "expr:Expr", message = "the '&' operator for taking addresses is no longer supported in Slang" } |
There was a problem hiding this comment.
🟡 Gap: Error message lacks migration guidance and user-facing docs need updating
The message tells users that & is no longer supported but doesn't suggest a replacement. The PR description says "Internal code uses __getAddress instead," and the parser does register __getAddress as a syntax keyword. Users encountering this error won't know how to fix their code.
Additionally, the user guide at docs/user-guide/03-convenience-features.md (lines 546–598, "Pointers (limited)" section) explicitly shows & operator usage for obtaining pointers:
- Line 558:
MyType* pNext2 = &pNext[1]; - Line 576:
return test(&obj); // !! ERROR !! - Line 592: "Slang can produce pointers using the & operator from data in global memory."
These docs become incorrect with this change and should be updated.
Suggestion: If __getAddress is the intended user-facing replacement, update the message to mention it:
span { loc = "expr:Expr", message = "the '&' operator for taking addresses is no longer supported in Slang; use '__getAddress(expr)' instead" }If __getAddress is internal-only (the __ prefix suggests this), consider what the user migration path is and document it in the user guide.
| @@ -0,0 +1,11 @@ | |||
| //DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK): -target spirv -entry computeMain -stage compute | |||
There was a problem hiding this comment.
🟡 Gap: Test only covers SPIRV target with one basic pattern
Since the error is emitted at the semantic checker level (target-independent), the -target spirv flag here is not strictly necessary. More importantly, the test only covers &buffer[threadId.x] (global buffer element access). Given this is a breaking change, additional coverage would strengthen confidence:
&localVar— local variable address-of (previously valid on CPU/CUDA targets)&structInstance.member— struct member access&in generic function bodies — ensure deprecation fires during specialization- A positive test showing
__getAddress(...)still works as the replacement
Suggestion: Consider removing the target flag (since the error is target-independent) or adding a second test variant for -cpu to confirm the error is consistent across backends.
The prefix
&operator for taking addresses has unclear semantics on GPU targets and is not intended to be part of the Slang language going forward.This change adds an unconditional error (30087: "the '&' operator for taking addresses is no longer supported in Slang") when the resolved function has
KnownBuiltinDeclName::OperatorAddressOf, emitted fromCheckInvokeExprWithCheckedOperands. Theoperator&declaration itself is unchanged. Internal code uses__getAddressinstead.